Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: feature: add support for wayland-security-context-v1 #6436

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP: feature: add support for wayland-security-context-v1 #6436

wants to merge 1 commit into from

Conversation

FelixPehla
Copy link
Contributor

@FelixPehla FelixPehla commented Aug 18, 2024 •
edited
Loading

Add support for limiting access to privileged wayland protocols via security-context-v1

To do this firejail needs to create a new wayland socket and attach a security context to it, which it then passes to the sandboxed application.

See also: flatpak#4920

Todo:

  • build:
    • add wayland-protocols and wayland-scanner as build dependencies when building with HAVE_WAYLAND_SECURITY_CONTEXT
  • profiles:
    • check which programs can run natively under wayland. Start by looking for programs whitelisting the wayland socket and go from there.
  • src:
    • man: document changes
    • firemon: list which wayland display is in use and information about the attached security context
    • bash_completion, zsh_completion: add --wayland cli flag
    • jailcheck: test for presence of security-context global and creating a security context for an application
    • firejail:
      • implement wayland filter (either wayland for using a security context, or wayland none to disable access to wayland)
      • create wayland security context and attach it to a process. Should be pretty simple, I already wrote a small program that does it here.

Open Questions:

  • what to do with WAYLAND_SOCKET, see flatpak#5614:
    • unset it by default and add a global setting to allow it, which can be done on DEs which use them (KDE)
    • unset it by default and use a special filter option and enable it in profiles that commonly use this feature (check on flathub for apps which enable it). (this is the one that I find the most sensible)
    • don't unset it by default
  • sandbox_engine to use. Flatpak uses org.flatpak, use com.wordpress.firejail, thanks @rusty-snake
  • app_id to use. Flatpak uses the flatpak application ID (e.g. org.signal.Signal for the Signal messenger app), maybe use the name of the profile in use?
  • instance_id to use. Has to be unique to the currently running sandbox and should never be reused. Just use a (secure) random identifier from /dev/urandom or similar?

@FelixPehla FelixPehla marked this pull request as draft August 18, 2024 13:06
@rusty-snake
Copy link
Collaborator

rusty-snake commented Aug 18, 2024 •
edited
Loading

sandbox_engine to use. Flatpak uses org.flatpak, maybe use org.firejail?

com.wordpress.firejail

app_id to use. Flatpak uses the flatpak application ID (e.g. org.signal.Signal for the Signal messenger app), maybe use the name of the profile in use?

Sounds good. But consider default.profile and --noprofile. There's also --name and the name of the binary.

For what is the name used? Does a random name make sense, problems?

instance_id to use. Has to be unique to the currently running sandbox and should never be reused. Just use a (secure) random identifier from /dev/urandom or similar?

Since pids can get reused, a random id (maybe /proc/sys/kernel/random/uuid) soudns good to me.

We do not have anything like a unique instance identifier (usually pids are used) in firejail yet, so it can not be used for anything else.

profiles: check which programs can run natively under wayland. Start by looking for programs whitelisting the wayland socket and go from there.

Adding a new feature (--wayland in that case) in a first PR and adding it to supported profiles is a code practise to keep PRs reviewable (also the reviewer might differ).

@FelixPehla
Copy link
Contributor Author

FelixPehla commented Aug 18, 2024 •
edited
Loading

sandbox_engine to use. Flatpak uses org.flatpak, maybe use org.firejail?

com.wordpress.firejail

makes sense

app_id to use. Flatpak uses the flatpak application ID (e.g. org.signal.Signal for the Signal messenger app), maybe use the name of the profile in use?

Sounds good. But consider default.profile and --noprofile. There's also --name and the name of the binary.

For what is the name used? Does a random name make sense, problems?

From the wayland protocol:

The application ID is an opaque, sandbox-specific identifier for an
application. See the well-known engines document for more details:
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/main/staging/security-context/engines.md

The compositor may use the application ID to group clients belonging to
the same security context application.

So it should not be random, and it should be the same for every instance of the running program, so it should probably not be the sandbox name either.

On second thought the profile name may not be suited for this as it does not identify the actually running program since you can just use --profile= to use a different one. Using the full path to the binary would work better, but it might change across OSes, and I'm not sure if some compositors have a maximum length for this that is lower than the maximum path lenght on common filesystems.

instance_id to use. Has to be unique to the currently running sandbox and should never be reused. Just use a (secure) random identifier from /dev/urandom or similar?

Since pids can get reused, a random id (maybe /proc/sys/kernel/random/uuid) soudns good to me.

We do not have anything like a unique instance identifier (usually pids are used) in firejail yet, so it can not be used for anything else.

It's mostly just a way for compositors to identify a running instance in a race free way, I would just use a secure random source that's easy to call.

profiles: check which programs can run natively under wayland. Start by looking for programs whitelisting the wayland socket and go from there.

Adding a new feature (--wayland in that case) in a first PR and adding it to supported profiles is a code practise to keep PRs reviewable (also the reviewer might differ).

Sounds reasonable. I'll keep it here as a reminder, for the time being and open a new PR if this one has been merged.

@FelixPehla FelixPehla mentioned this pull request Aug 18, 2024
Open
@kmk3 kmk3 added the enhancement New feature request label Aug 25, 2024
@kmk3 kmk3 changed the title WIP: add support for wayland-security-context-v1 WIP: feature: add support for wayland-security-context-v1 Aug 25, 2024
kmk3
kmk3 reviewed Aug 27, 2024
View reviewed changes
src/firejail/checkcfg.c
Comment on lines +427 to +432
#endif
"\n\t- wayland security context support is "
#ifdef HAVE_WAYLAND_SECURITY_CONTEXT
"enabled"
#else
"disabled"
Copy link
Collaborator

@kmk3 kmk3 Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default put the wayland code right before the x11 code so that it's more or
less sorted.

This applies to all files except wayland.c (and env.c, which is already sorted).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmk3 kmk3 kmk3 left review comments

Assignees
No one assigned
Labels
enhancement New feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FelixPehla @rusty-snake @kmk3